Skip to content

Refactor nginx config in prod #7417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maruiz93
Copy link
Contributor

Follow up of 7033 PR

After checking the changes in staging work as expected.

@openshift-ci openshift-ci bot requested review from filariow and rrosatti July 31, 2025 19:55
Copy link

openshift-ci bot commented Jul 31, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: maruiz93
Once this PR has been reviewed and has the lgtm label, please assign sadlerap for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@maruiz93
Copy link
Contributor Author

maruiz93 commented Jul 31, 2025

Follow up of 7033 PR

Signed-off-by: Marta Anon <[email protected]>
@maruiz93 maruiz93 force-pushed the refactor-nginx-configs-prod branch from e74384f to 848a6a1 Compare August 14, 2025 08:23
Copy link
Contributor

Code Review by Gemini

The changes introduce a Nginx configuration syntax error and remove the flexibility to configure backend URLs (Tekton Results, KubeArchive) via environment variables, which is a regression in configurability.

Here are the identified issues and suggested improvements:

1. Nginx Configuration Syntax Error (Nested Location Blocks)

Issue:
The file components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf attempts to include tekton-results.conf. However, tekton-results.conf (as introduced in this PR) is itself a full location block. Nginx does not allow location blocks to be nested or included within other location blocks in this manner. This will result in a Nginx configuration error.

Files to change:

  • components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf
  • components/konflux-ui/production/base/proxy/tekton-results.conf
  • components/konflux-ui/production/base/proxy/kubearchive.conf

Reasoning for change:
The include directive within a location block is typically used to include directives (like proxy_pass, proxy_set_header, etc.), not entire location blocks. The original setup had a dynamically generated file containing only the proxy_pass directive. This pattern should be restored.

Suggested Change:

--- a/components/konflux-ui/production/base/proxy/kubearchive.conf
+++ b/components/konflux-ui/production/base/proxy/kubearchive.conf
@@ -2,5 +2,5 @@
     auth_request /oauth2/auth;
     rewrite /api/k8s/plugins/kubearchive/(.+) /$1 break;
     proxy_read_timeout 30m;
-    proxy_pass https://kubearchive-api-server.product-kubearchive.svc.cluster.local:8081;
+    include /mnt/nginx-generated-config/kubearchive-proxy-pass.conf;
     include /mnt/nginx-generated-config/auth.conf;
 }
--- a/components/konflux-ui/production/base/proxy/tekton-results.conf
+++ b/components/konflux-ui/production/base/proxy/tekton-results.conf
@@ -3,6 +3,6 @@
 
     rewrite /api/k8s/plugins/tekton-results/(.+) /$1 break;
     proxy_read_timeout 30m;
-    proxy_pass https://tekton-results-api-service.tekton-results.svc.cluster.local:8080;
+    include /mnt/nginx-generated-config/tekton-results-proxy-pass.conf;
     include /mnt/nginx-generated-config/auth.conf;
 }
--- a/components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf
+++ b/components/konflux-ui/production/base/proxy/tekton-results-workspaces.conf
@@ -4,6 +4,6 @@
 
     rewrite /api/k8s/plugins/tekton-results/workspaces/.+?/(.+) /$1 break;
     proxy_read_timeout 30m;
-    include /mnt/nginx-generated-config/tekton-results.conf;
+    include /mnt/nginx-generated-config/tekton-results-proxy-pass.conf;
     include /mnt/nginx-generated-config/auth.conf;
 }

2. Loss of Configurability (Hardcoded URLs and IMPERSONATE flag)

Issue:
The proxy-init-config ConfigMap, which provided environment-specific URLs for Tekton Results and KubeArchive, has been removed. The generate-nginx-configs init container no longer uses these environment variables to dynamically generate the proxy_pass directives. Instead, the URLs are now hardcoded directly into the Nginx config files (kubearchive.conf, tekton-results.conf). This removes the ability to easily configure these URLs per environment using Kustomize overlays. Additionally, the conditional generation of auth.conf based on the IMPERSONATE flag was removed, making impersonation always enabled.

Files to change:

  • components/konflux-ui/production/base/proxy/kustomization.yaml
  • components/konflux-ui/production/base/proxy/proxy.yaml
  • components/konflux-ui/production/kflux-ocp-p01/kustomization.yaml (and similar files for other environments like kflux-osp-p01, kflux-prd-rh02, etc.)
  • components/konflux-ui/production/kflux-osp-p01/kubearchive.conf (and similar files for other environments)

Reasoning for change:
To restore configurability and maintain the dynamic generation of proxy_pass directives based on environment variables. The IMPERSONATE flag logic should also be restored for auth.conf generation.

Suggested Change:

a) Reintroduce proxy-init-config in base kustomization.yaml:

--- a/components/konflux-ui/production/base/proxy/kustomization.yaml
+++ b/components/konflux-ui/production/base/proxy/kustomization.yaml
@@ -5,9 +5,12 @@
 configMapGenerator:
   - name: proxy
     files:
       - nginx.conf
   - name: proxy-nginx-templates
     files:
       - auth.conf
   - name: proxy-nginx-static
     files:
       - tekton-results.conf
       - tekton-results-workspaces.conf
       - kubearchive.conf
+  - name: proxy-init-config
+    literals:
+      - IMPERSONATE=true
+      - TEKTON_RESULTS_URL=https://tekton-results-api-service.tekton-results.svc.cluster.local:8080
+      - KUBEARCHIVE_URL=https://kubearchive-api-server.product-kubearchive.svc.cluster.local:8081

b) Modify proxy.yaml to use proxy-init-config and generate proxy_pass files:

--- a/components/konflux-ui/production/base/proxy/proxy.yaml
+++ b/components/konflux-ui/production/base/proxy/proxy.yaml
@@ -47,37 +47,39 @@
         resources:
           limits:
             cpu: 50m
             memory: 128Mi
           requests:
             cpu: 10m
             memory: 64Mi
       - name: generate-nginx-configs
         image: registry.access.redhat.com/ubi9/ubi@sha256:66233eebd72bb5baa25190d4f55e1dc3fff3a9b77186c1f91a0abdb274452072
-        envFrom:
-          - configMapRef:
-              name: proxy-init-config
+        envFrom:
+          - configMapRef:
+              name: proxy-init-config
         command:
           - sh
           - -c
           - |
             set -e
 
-            # Generate auth.conf with bearer token replacement
+            # Generate auth.conf with bearer token replacement (conditional on IMPERSONATE)
             token=$(cat /mnt/api-token/token)
-            sed "s/__BEARER_TOKEN__/$token/g" /mnt/nginx-templates/auth.conf > /mnt/nginx-generated-config/auth.conf
+            if [[ "$IMPERSONATE" == "true" ]]; then
+              sed "s/__BEARER_TOKEN__/$token/g" /mnt/nginx-templates/auth.conf > /mnt/nginx-generated-config/auth.conf
+            else
+              echo "# impersonation was disabled by config" > /mnt/nginx-generated-config/auth.conf
+            fi
 
             chmod 640 /mnt/nginx-generated-config/auth.conf
 
-            chmod 640 "$auth_conf"
-
-            echo \
-              "proxy_pass ${TEKTON_RESULTS_URL:?tekton results url must be provided};" \
-              > /mnt/nginx-generated-config/tekton-results.conf
-
-            if [[ "$KUBEARCHIVE_URL" != "" ]]; then
-              echo "location /api/k8s/plugins/kubearchive/ {" > /mnt/nginx-generated-config/kubearchive.conf
-              echo "auth_request /oauth2/auth;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "rewrite /api/k8s/plugins/kubearchive/(.+) /\$1 break;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "proxy_read_timeout 30m;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "proxy_pass ${KUBEARCHIVE_URL};" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "include /mnt/nginx-generated-config/auth.conf;" >> /mnt/nginx-generated-config/kubearchive.conf
-              echo "}" >> /mnt/nginx-generated-config/kubearchive.conf
+            # Generate tekton-results-proxy-pass.conf
+            echo "proxy_pass ${TEKTON_RESULTS_URL:?tekton results url must be provided};" \
+              > /mnt/nginx-generated-config/tekton-results-proxy-pass.conf
+
+            # Generate kubearchive-proxy-pass.conf
+            if [[ "$KUBEARCHIVE_URL" != "" ]]; then
+              echo "proxy_pass ${KUBEARCHIVE_URL};" \
+                > /mnt/nginx-generated-config/kubearchive-proxy-pass.conf
             else
-              echo "# KubeArchive disabled by config" > /mnt/nginx-generated-config/kubearchive.conf
+              echo "# KubeArchive disabled by config" > /mnt/nginx-generated-config/kubearchive-proxy-pass.conf
             fi
 
         volumeMounts:
         - name: nginx-generated-config
           mountPath: /mnt/nginx-generated-config
         - name: nginx-templates
           mountPath: /mnt/nginx-templates
         - name: api-token
           mountPath: /mnt/api-token
         securityContext:
           readOnlyRootFilesystem: true
           runAsNonRoot: true
           runAsUser: 1001
         resources:
           limits:
             cpu: 50m
             memory: 128Mi

c) Correct Kustomize overlays for environment-specific configurations:

For environments where KubeArchive is disabled (e.g., kflux-osp-p01, kflux-prd-rh02, stone-prd-rh01, stone-prod-p01), the proxy-init-config should be merged to set KUBEARCHIVE_URL to an empty string, rather than removing proxy-init-config and providing a commented kubearchive.conf.

Example for components/konflux-ui/production/kflux-osp-p01/kustomization.yaml:

--- a/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
+++ b/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
@@ -1,24 +1,24 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
   - ../base
   - configure-oauth-proxy-secret.yaml
 
 configMapGenerator:
   - name: dex
     files:
       - dex-config.yaml
-  - name: proxy-init-config
-    literals:
-      - IMPERSONATE=true
-      - TEKTON_RESULTS_URL=https://tekton-results-api-service.tekton-results.svc.cluster.local:8080
+  - name: proxy-init-config
+    literals:
+      - KUBEARCHIVE_URL= # Set to empty to disable KubeArchive
+    behavior: merge
   - name: proxy-nginx-static
     files:
       - kubearchive.conf
     behavior: merge
 
 patches:
   - path: add-service-certs-patch.yaml
     target:
       group: ""
       version: v1

Note: The proxy-nginx-static block for kubearchive.conf in these overlays becomes redundant if KUBEARCHIVE_URL is set to empty, as the generate-nginx-configs script will handle disabling it. It can be removed from these overlays for cleaner configuration.

Example for components/konflux-ui/production/kflux-osp-p01/kustomization.yaml (cleaner version):

--- a/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
+++ b/components/konflux-ui/production/kflux-osp-p01/kustomization.yaml
@@ -1,24 +1,21 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
   - ../base
   - configure-oauth-proxy-secret.yaml
 
 configMapGenerator:
   - name: dex
     files:
       - dex-config.yaml
-  - name: proxy-init-config
-    literals:
-      - IMPERSONATE=true
-      - TEKTON_RESULTS_URL=https://tekton-results-api-service.tekton-results.svc.cluster.local:8080
+  - name: proxy-init-config
+    literals:
+      - KUBEARCHIVE_URL= # Set to empty to disable KubeArchive
+    behavior: merge
 
 patches:
   - path: add-service-certs-patch.yaml
     target:
       group: ""
       version: v1
       kind: Service
       name: proxy

And remove the corresponding kubearchive.conf files from these overlay directories (e.g., components/konflux-ui/production/kflux-osp-p01/kubearchive.conf).

Example for components/konflux-ui/production/kflux-osp-p01/kubearchive.conf:

--- a/components/konflux-ui/production/kflux-osp-p01/kubearchive.conf
+++ /dev/null
@@ -1 +0,0 @@
-# KubeArchive disabled by config

(This file should be deleted)

Apply similar changes to all other affected kustomization.yaml files and their corresponding kubearchive.conf files if they are meant to disable KubeArchive.

@maruiz93
Copy link
Contributor Author

/unhold

@maruiz93
Copy link
Contributor Author

@gbenhaim @sahil143 can I get your reviews here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants